-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[crater] Show per-table deltas in diff detail view #938
Conversation
398d4c9
to
3848727
Compare
fontc_crater/src/ci/html.rs
Outdated
fn make_delta_decoration<T: PartialOrd + Copy + Sub<Output = T> + Display + Default>( | ||
current: T, | ||
prev: Option<T>, | ||
more_is_better: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest enum instead of bool, the callsites are a bit opaque. A trait might be nice? - ratio.delta_decoration(prev, Better::More)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the enum, trait is definitely an option but would rather just get this merged than noodle on it much more. :)
fontc_crater/src/ci/html.rs
Outdated
None | Some(std::cmp::Ordering::Greater) if more_is_better => { | ||
html! { span.better { (diff) } } | ||
} | ||
Some(std::cmp::Ordering::Less) if !more_is_better => html! { span.better { (diff) } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be |'d onto the case above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there can only be a single if
clause on a match arm, and it applies after the match (it is not associated with a specific match item, see https://doc.rust-lang.org/1.80.1/reference/expressions/match-expr.html#match-expressions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL. Or re-learned I suppose since I'm sure I did read that at some point.
That is, in the detail view where it shows the per-table diff percentages for a given font, if there have been any changes from the previous run display the delta at the per-table level, so we can better understand what changed.
3848727
to
8175de3
Compare
That is, in the detail view where it shows the per-table diff percentages for a given font, if there have been any changes from the previous run display the delta at the per-table level, so we can better understand what changed.
You can see how this looks at https://googlefonts.github.io/fontc_crater/